Add OpenMP kernels by mcbarton · Pull Request #389 · compiler-research/xeus-cpp

mcbarton · 2025-10-07T13:31:33Z

Description

Please include a summary of changes, motivation and context for this PR.

Fixes # (issue)

Type of change

Please tick all options which are relevant.

Bug fix
New feature
Added/removed dependencies
Required documentation updates

codecov-commenter · 2025-10-07T13:35:33Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.94%. Comparing base (c357fd6) to head (903e597).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #389   +/-   ##
=======================================
  Coverage   81.94%   81.94%           
=======================================
  Files          21       21           
  Lines         853      853           
  Branches       87       87           
=======================================
  Hits          699      699           
  Misses        154      154

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mcbarton · 2025-10-09T08:57:09Z

Things done so far

Add c++ + openmp kernels (includes kernel definition json files added, and cmake change)
Updated documentation so that LD_LIBRARY_PATH is defined
Add example c++ + openmp notebooks which have been tested locally to work (taken from xeus-clang-repl)

List of things still to do

Add c + openmp kernels
Update ci so LD_LIBRARY_PATH is defined and openmp kernels can be tested
Add some tests for the openmp kernels

vgvassilev · 2025-10-09T10:58:34Z

We should probably have a openmp specific ci without having to rely on the ld_library_path..

mcbarton · 2025-10-09T11:02:30Z

We should probably have a openmp specific ci without having to rely on the ld_library_path..

@vgvassilev Having an openmp specific ci will not avoid the need for ld_library_path. To avoid ld_library_path you would need to rework the cmake and the kernelspec (which makes reference to this variable) , and the example notebooks would need to know the path where you stored the openmp library. Using ld_library_path like xeus-clang-repl did simplifies this PR.

vgvassilev · 2025-10-09T15:55:44Z

We should probably have a openmp specific ci without having to rely on the ld_library_path..

@vgvassilev Having an openmp specific ci will not avoid the need for ld_library_path. To avoid ld_library_path you would need to rework the cmake and the kernelspec (which makes reference to this variable) , and the example notebooks would need to know the path where you stored the openmp library. Using ld_library_path like xeus-clang-repl did simplifies this PR.

Okay, let's see if that's compatible with the vision of @SylvainCorlay which he expressed recently.

mcbarton · 2025-10-09T19:23:50Z

test/test_xcpp_kernel.py

+        #include <omp.h>
+        #include <iostream>
+        #include <clang/Interpreter/CppInterOp.h>
+        Cpp::LoadLibrary("libomp");


Cpp::LoadLibrary("libomp"); works in the notebooks, but doesn't appear to be working in the python kernel tests. I believe this because the tests fail with the same error that I get if I remove this line from the notebook (see https://github.com/compiler-research/xeus-cpp/actions/runs/18385849945/job/52383968040?pr=389#step:13:1015 for error). @vgvassilev @anutosh491 Any idea what may be going on?

My only guess is that the kernelspecs have the flag -fopenmp=libomp , but the python output has the fopenmp flag without the =libomp (see https://github.com/compiler-research/xeus-cpp/actions/runs/18385849945/job/52383968040?pr=389#step:13:922 ). I don't know where to change it so that it has -fopenmp=libomp too though.

Can you call the Cpp::LoadLibrary("libomp") in a separate declare operation?

I don't know if you was suggesting splitting this code into 2 executions, one which loads the library, and one which executes the openmp code, but that has worked, and we now have a passing test. Its a simple OpenMP test, but hopefully it will suffice.

mcbarton · 2025-10-10T08:00:37Z

@vgvassilev @alexander-penev @Vipul-Cariappa @anutosh491 @SylvainCorlay I now consider this PR ready for review. It adds c and c++ kernels that have openmp. It includes example notebooks for the c++ + openmp kernels taken from xeus-clang-repl which were able to run when I tried locally. It also has one simple openmp test for the jupyter kernel test tests. I wasn't exactly sure what would suffice as an automatic test, but the test clearly demonstrates the kernel is executing OpenMP code.

Vipul-Cariappa

According to these changes; The user after selecting a omp kernel, still needs to include CppInterOp.h and call Cpp::LoadLibrary(...). The user should not be expected to know the internals and do this.
When we launch a kernel that enables OpenMP, it should automatically load all necessary stuff. You need to parse the compiler arguments, check if it contains -fopenmp, if yes, call LoadLibrary.
Also, can you share the contents of kernel.json files that gets installed.

Stretch goal (may not be possible): Can we get it to work, without modifying the LD_LIBRARY_PATH env var?

mcbarton · 2025-10-14T20:45:26Z

I think I have an idea of how to avoiding needing ld library path. My new method should work on the Windows once we have that platform working too. I just need to test it out, and plan to test it out sometime in the next few days.

I'm not so convinced that making the user have to include CppInterOp.h, and calling cpp::LoadLibrary(...) is as big deal as you make out to be. It would also let the user know to use other (non openmp) shared libraries in their notebooks. The example notebooks are there for a reason. I kept this PR the way xeus-clang-repl did openmp kernels. Despite this I will look into working out to have the kernel automatically do this if it notices -fopenmp in the kernel arguments.

CMakeLists.txt

github-actions

clang-tidy made some suggestions

github-actions · 2026-01-29T14:54:00Z

src/xinterpreter.cpp

  // FIXME: We should process the kernel input options and conditionally pass
  // the gpu args here.
-  return Cpp::CreateInterpreter(ClangArgs/*, {"-cuda"}*/);
+  Cpp::TInterp_t res = Cpp::CreateInterpreter(ClangArgs /*, {"-cuda"}*/);


warning: no header providing "CppImpl::CreateInterpreter" is directly included [misc-include-cleaner]

Cpp::TInterp_t res = Cpp::CreateInterpreter(ClangArgs /*, {"-cuda"}*/); ^

github-actions · 2026-01-29T14:54:00Z

src/xinterpreter.cpp

  // FIXME: We should process the kernel input options and conditionally pass
  // the gpu args here.
-  return Cpp::CreateInterpreter(ClangArgs/*, {"-cuda"}*/);
+  Cpp::TInterp_t res = Cpp::CreateInterpreter(ClangArgs /*, {"-cuda"}*/);


warning: no header providing "CppImpl::TInterp_t" is directly included [misc-include-cleaner]

Cpp::TInterp_t res = Cpp::CreateInterpreter(ClangArgs /*, {"-cuda"}*/); ^

github-actions · 2026-01-29T14:54:00Z

src/xinterpreter.cpp

+      )
+      != ClangArgs.end())
+  {
+      Cpp::LoadLibrary("libomp");


warning: no header providing "CppImpl::LoadLibrary" is directly included [misc-include-cleaner]

Cpp::LoadLibrary("libomp"); ^

AntoinePrv · 2026-02-03T17:00:12Z

Should we take the changes to the existing kernels and the CMakeLists.txt changes not related to openMP in a separate PR and merge that?

I am interested in these changes but I see this PR is opened since october.

mcbarton · 2026-02-03T21:39:43Z

Should we take the changes to the existing kernels and the CMakeLists.txt changes not related to openMP in a separate PR and merge that?

I am interested in these changes but I see this PR is opened since october.

@AntoinePrv This PR has been open for a while because certain issues needed to be resolved. I got the OpenMP kernels working, but it required a hack due to issue with CppInterOp (see this issue compiler-research/CppInterOp#784). The reason ci is currently red is because I prepared it for a new CppInterOp release which has a patch created by @Vipul-Cariappa in CppInterOp to workaround the issue.

cc @vgvassilev

vgvassilev · 2026-02-04T06:42:10Z

CMakeLists.txt

-    set(XEUS_CPP_LD_LIBRARY_PATH "$ENV{LD_LIBRARY_PATH}")
    set(XEUS_CPP_INCLUDE_DIR ${CMAKE_INSTALL_PREFIX}/include)
+    if(${kernel} MATCHES "omp/$")
+      set(XEUS_CPP_OMP "${OpenMP_CXX_FLAGS}")


Do we still need this? Is that the include path to openmp? I feel uncomfortable exporting that information at build time whereas it's needed at runtime and we should be able to resolve that then. However, that's not a new problem per se because we already to this for other things..

vgvassilev

LGTM! Consider squash on merge.

anutosh491

I'll be really honest, I'm not convinced with this.

Please go through this comment of mine : #432 (comment) (especially point 2)

Just the idea of enabling OpenMP by default is not sitting well with me

https://github.com/compiler-research/xeus-cpp/pull/389/files#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR110

Sorry for my limited capacity to look into this today (as I'm Brussels after FOSDEM & shall reach back home in India tomorrow night) but commenting at a surface level I'll just say an openmp kernel looks like a specific use case to me. And changes to the environment file/CMakelists for finding relevant paths should only be done if the user ends up wanting to build an openmp kernel right ?

I wouldn't mind it if is was not bringing in changes to the "default"

For eg all wasm kernels are SIMD enabled (which technically is a specific use case)

xeus-cpp/share/jupyter/kernels/xcpp23/wasm_kernel.json.in

Line 8 in 12c4570

"-mllvm", "-wasm-enable-sjlj", "-msimd128"

but it doesn't need any extra cmake changes whatsoever.

I would really like @JohanMabille or @SylvainCorlay to look at this before we take this in.

Also sorry again for missing any discussion done above as to how we proceed in the future but I see the CI is red too, so not sure we can merge.

If the parameterized JEP is the solution for handling this better: jupyter/enhancement-proposals#87
Let's just use it !

But if it's not and we if just go ahead with this and refactor later, I would like to atleast see an issue on github as to what the plan is !

vgvassilev · 2026-02-04T08:06:04Z

I'll be really honest, I'm not convinced with this.

Please go through this comment of mine : #432 (comment) (especially point 2)

I share the same sentiment, however, there is practically no way to do this as of today. We have been waiting for a solution to this issue since 2021 and I do not think it is reasonable to drop major features like openmp for this reason unfortunately.

Just the idea of enabling OpenMP by default is not sitting well with me

https://github.com/compiler-research/xeus-cpp/pull/389/files#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR110

Sorry for my limited capacity to look into this today (as I'm Brussels after FOSDEM & shall reach back home in India tomorrow night) but commenting at a surface level I'll just say an openmp kernel looks like a specific use case to me. And changes to the environment file/CMakelists for finding relevant paths should only be done if the user ends up wanting to build an openmp kernel right ?

I wouldn't mind it if is was not bringing in changes to the "default"

Blocking this would also block the conda recipe and other changes proposed by Antoine because I believe they will cause more friction when this work resumes/goes in. In fact, we are moving forward and had a few emergency meetings so that we don't have to block Antoine's work and the current state of the PR is good enough. I agree this cannot go forever but that's not a good argument to disallow omp kernels -- that's one of the best use cases to teach parallel programming with xeus-cpp.

For eg all wasm kernels are SIMD enabled (which technically is a specific use case)

xeus-cpp/share/jupyter/kernels/xcpp23/wasm_kernel.json.in

Line 8 in 12c4570

"-mllvm", "-wasm-enable-sjlj", "-msimd128"

but it doesn't need any extra cmake changes whatsoever.

I would really like @JohanMabille or @SylvainCorlay to look at this before we take this in.

Also sorry again for missing any discussion done above as to how we proceed in the future but I see the CI is red too, so not sure we can merge.

If the parameterized JEP is the solution for handling this better: jupyter/enhancement-proposals#87 Let's just use it !

But if it's not and we if just go ahead with this and refactor later, I would like to atleast see an issue on github as to what the plan is !

I would be more than happy to drop all kernels when this becomes widely available. @mcbarton can you open an issue to track this?

anutosh491

Okay then, let's track the development here through an issue and then let's take this in !

Thank you for the explanation :)

JohanMabille · 2026-02-04T08:41:08Z

It seems that we all agree that this solution is not ideal but we don't really have an alternative for now. Also this does not scale on the long run (supporting CUDA would reqiure 6 more kernels and additional dependencies for instance).

I have good hope that the parametrized kernelspec JEP will be merged soon and I'll tackle its implementation in xeus right after. I think we'll also need to add a plugin system so that the additional required code (for loading libraries / initializing components / whatever) can live out of the "core" kernel.

That being said, I don't think this PR is ready to merge because of the CI failures. The failing jobs should be fixed before we merge this.

Vipul-Cariappa

I too believe the CMake changes may not be required. At least the way it is implemented now. But we can track it in an issue, and fix it later.
Reasoning: OpenMP is not used by xeus-cpp itself (at least at build time), it is just an optional runtime dependency to the interpreter instance. Nowhere in xeus-cpp code we have #include <omp.h> ...

Otherwise, LGTM!

SylvainCorlay · 2026-02-04T09:36:22Z

On the packaging end, we will probably split the conda recipe in multiple outputs (so that installing xeus-cpp does not include the OpenMP kernelspecs unless e.g. xeus-cpp-openmp is installed).

I would also like to highlight that the Jupyter Enhancement Proposal was specifically created to address the C++ kernel use case. It has undergone extensive reviews, and our team produced a reference implementation covering kernel, server, and frontend components. The JEP is currently in the final voting phase.

Given that the official standardization process is nearly complete, it is disappointing to rush a workaround while the long-term solution is so close to being finalized and was developed specifically with the C++ kernel in mind.

vgvassilev · 2026-02-04T09:55:25Z

On the packaging end, we will probably split the conda recipe in multiple outputs (so that installing xeus-cpp does not include the OpenMP kernelspecs unless e.g. xeus-cpp-openmp is installed).

I would also like to highlight that the Jupyter Enhancement Proposal was specifically created to address the C++ kernel use case. It has undergone extensive reviews, and our team produced a reference implementation covering kernel, server, and frontend components. The JEP is currently in the final voting phase.

Given that the official standardization process is nearly complete, it is disappointing to rush a workaround while the long-term solution is so close to being finalized and was developed specifically with the C++ kernel in mind.

Do we have an ETA for this?

…ion on macos

mcbarton force-pushed the Add-OpenMP-kernel branch 3 times, most recently from e4af2d6 to 3824a2f Compare October 9, 2025 07:23

vgvassilev requested review from Vipul-Cariappa, alexander-penev and anutosh491 October 9, 2025 07:37

mcbarton force-pushed the Add-OpenMP-kernel branch from 7dda968 to cc7f3c2 Compare October 9, 2025 08:55

mcbarton force-pushed the Add-OpenMP-kernel branch from 6c4d0f4 to dbe5005 Compare October 9, 2025 10:54

mcbarton force-pushed the Add-OpenMP-kernel branch 3 times, most recently from 45a4ee5 to 571023c Compare October 9, 2025 12:43

vgvassilev requested a review from SylvainCorlay October 9, 2025 15:55

mcbarton force-pushed the Add-OpenMP-kernel branch from 1feccb1 to 1b1b9cd Compare October 9, 2025 18:44

mcbarton commented Oct 9, 2025

View reviewed changes

mcbarton force-pushed the Add-OpenMP-kernel branch 2 times, most recently from 5d1ad18 to 5a2630d Compare October 10, 2025 07:47

Vipul-Cariappa requested changes Oct 14, 2025

View reviewed changes

mcbarton force-pushed the Add-OpenMP-kernel branch 3 times, most recently from 2dac7b4 to a41ed55 Compare October 15, 2025 12:34

mcbarton commented Oct 15, 2025

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

mcbarton requested review from Vipul-Cariappa and vgvassilev October 15, 2025 17:59

Apply suggestion now that CppInterOp v1.8.0 is out

c805b83

mcbarton force-pushed the Add-OpenMP-kernel branch from 56bb93a to c805b83 Compare January 29, 2026 14:47

github-actions bot reviewed Jan 29, 2026

View reviewed changes

mcbarton and others added 5 commits January 29, 2026 16:47

Try Vipul solution to failing bot

fcecfc1

Reverse order of asserts

599fd79

Remove unneeded compiler-rt package

f0b08a6

Remove unneccessary logic in the cmake file

7967c64

Remove unneeded XEUS_CPP_PATH and XEUS_CPP_LD_LIBRARY_PATH

71c1764

vgvassilev reviewed Feb 4, 2026

View reviewed changes

vgvassilev approved these changes Feb 4, 2026

View reviewed changes

anutosh491 requested changes Feb 4, 2026

View reviewed changes

anutosh491 approved these changes Feb 4, 2026

View reviewed changes

Vipul-Cariappa approved these changes Feb 4, 2026

View reviewed changes

mcbarton added 2 commits February 4, 2026 12:10

Change cxx-compiler to clangxx to enable resource directory installat…

eaca6a2

…ion on macos

Add curl dependency

ff4db5f

mcbarton force-pushed the Add-OpenMP-kernel branch from 11473b1 to ff4db5f Compare February 4, 2026 12:16

mcbarton added 3 commits February 4, 2026 12:22

Try linking to curl in a different way

8ccc565

Revert some changes relating to curl

ed91280

find curl unix systems

3991440

mcbarton force-pushed the Add-OpenMP-kernel branch 2 times, most recently from d17cf3e to 58eaaa7 Compare February 4, 2026 12:55

Test fix

f7552c9

mcbarton force-pushed the Add-OpenMP-kernel branch from a889c1b to f7552c9 Compare February 4, 2026 13:00

Fix for Windows

a2224a8

Conversation

mcbarton commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Uh oh!

codecov-commenter commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mcbarton commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vgvassilev commented Oct 9, 2025

Uh oh!

mcbarton commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vgvassilev commented Oct 9, 2025

Uh oh!

mcbarton Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcbarton Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

vgvassilev Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

mcbarton Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcbarton commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Vipul-Cariappa left a comment

Choose a reason for hiding this comment

Uh oh!

mcbarton commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

AntoinePrv commented Feb 3, 2026

Uh oh!

mcbarton commented Feb 3, 2026

Uh oh!

vgvassilev Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

vgvassilev left a comment

Choose a reason for hiding this comment

Uh oh!

anutosh491 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vgvassilev commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anutosh491 left a comment

Choose a reason for hiding this comment

Uh oh!

JohanMabille commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Vipul-Cariappa left a comment

mcbarton commented Oct 7, 2025 •

edited

Loading

codecov-commenter commented Oct 7, 2025 •

edited

Loading

mcbarton commented Oct 9, 2025 •

edited

Loading

mcbarton commented Oct 9, 2025 •

edited

Loading

mcbarton Oct 9, 2025 •

edited

Loading

mcbarton Oct 10, 2025 •

edited

Loading

mcbarton commented Oct 10, 2025 •

edited

Loading

mcbarton commented Oct 14, 2025 •

edited

Loading

anutosh491 left a comment •

edited

Loading

vgvassilev commented Feb 4, 2026 •

edited

Loading

JohanMabille commented Feb 4, 2026 •

edited

Loading